-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests: Support multiple keys in linearizability tests #14924
tests: Support multiple keys in linearizability tests #14924
Conversation
bf1c9f6
to
b4e9dc1
Compare
Codecov Report
@@ Coverage Diff @@
## main #14924 +/- ##
==========================================
- Coverage 74.92% 74.55% -0.38%
==========================================
Files 415 415
Lines 34276 34276
==========================================
- Hits 25682 25554 -128
- Misses 6970 7072 +102
- Partials 1624 1650 +26
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Resolved all comments that do not make sense. Please describe what you would like to change and not send 20 suggestions that will not work. Suggestions make sense only if:
|
b4e9dc1
to
59b34b2
Compare
tests/linearizability/model.go
Outdated
i++ | ||
} | ||
} | ||
state.PossibleValues = state.PossibleValues[:i] | ||
states = states[:i] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clever idea with the state reduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, but looks like it's not very readable. Rewrote it to make the code cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm (non-binding)
f7f440a
to
2cd4fba
Compare
Overall looks good to me. The only concern is the documentation. The model is the most important part of the linearizablity test, so please add detailed document/comment to describe the overall thought and make sure the document/comment is always up-to-date, especially you are changing the code too fast and frequently. I should raise this comment in your last PR in which you totally changed the implementation. Otherwise others may spend lots of time to understand your thoughts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2cd4fba
to
e6d44e4
Compare
Decided to separate PR that refactors the model first. Let's discuss documenting model there. #15045 |
55fd9b1
to
7b891f9
Compare
Please add self-contained comment in
|
7b891f9
to
0d5cd8a
Compare
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
8e181b2
to
ab093f5
Compare
@ahrtr Done, please take a look. |
ab093f5
to
818fb88
Compare
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com> Co-authored-by: Benjamin Wang <wachao@vmware.com>
10e4d8f
to
d4c8611
Compare
cc @ahrtr @spzala @tjungblu